-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 ClusterCacheTracker: drop unused Log field #6318
🌱 ClusterCacheTracker: drop unused Log field #6318
Conversation
lgtm, technically incompatible change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should deprecate the field in the struct for a release before removing, right? We can remove the use of the field through the code though.
Note: we can do breaking changes in golang between minors, it is just a matter to document them (only API types and CLI have different guarantees) |
Given that this type is likely to be set outside the codebase I think deprecation and removal at the next version update would be better for consumers, even if outright removal is allowed. |
Yup looks like based on our guidelines I have to deprecate it in a minor release first: https://github.com/kubernetes-sigs/cluster-api/blob/88b5a8edc855775a2545b61d30b1470c6b21f26d/CONTRIBUTING.md#codebase-and-go-modules I'll change the PR to still drop all usages but only deprecate the field in the struct. |
Signed-off-by: Stefan Büringer buringerst@vmware.com
682af14
to
4ce8299
Compare
@@ -44,6 +45,7 @@ type ClusterCacheReconciler struct { | |||
|
|||
func (r *ClusterCacheReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { | |||
err := ctrl.NewControllerManagedBy(mgr). | |||
Named("remote/clustercache"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabriziopandini I set the name here.
This will lead to a "controller": "remote/clustercache" k/v pair.
Before this PR the logs from this controller had "controller": "cluster" (same as the cluster controller).
Note: This info is based on controller-runtime main. The current v0.11 release will only use the name as prefix for messages ("controller/cluster: <msg>").
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Stefan Büringer buringerst@vmware.com
What this PR does / why we need it:
As we are always using the logger from context we don't have to set the Log field anymore, so let's drop it.
Note: before this PR the fields was written a few times, but never read.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #